Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix MCMC non-deterministic seeding with Generators #7637

Merged
merged 8 commits into from
Jan 7, 2025

Conversation

ricardoV94
Copy link
Member

@ricardoV94 ricardoV94 commented Jan 7, 2025

Closes #7612

Picked up the fix from #7540


📚 Documentation preview 📚: https://pymc--7637.org.readthedocs.build/en/7637/

@ricardoV94 ricardoV94 requested a review from lucianopaz January 7, 2025 16:18
@ricardoV94 ricardoV94 changed the title Fix mcmc Generator seeding Fix mcmc non-deterministic seeding with Generators Jan 7, 2025
@ricardoV94 ricardoV94 changed the title Fix mcmc non-deterministic seeding with Generators Fix MCMC non-deterministic seeding with Generators Jan 7, 2025
Copy link
Contributor

@lucianopaz lucianopaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@lucianopaz lucianopaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The failing test from the mac env actually happens because your second commit doesn't have all of the changes to pymc.step_methods.hmc.quadpotential that I had over in my PR

@ricardoV94
Copy link
Member Author

The failing test from the mac env actually happens because your second commit doesn't have all of the changes to pymc.step_methods.hmc.quadpotential that I had over in my PR

So there was a broken intermediate commit?

@ricardoV94
Copy link
Member Author

I guess this is missing: 2913e4a

@ricardoV94
Copy link
Member Author

It's strange though, the test passes locally for me

@ricardoV94
Copy link
Member Author

Ah must be the other mp_ctx that Mac defaults to. Should we be worried though that is not being passed?

@lucianopaz
Copy link
Contributor

lucianopaz commented Jan 7, 2025

I guess this is missing: 2913e4a

Yes, it's that commit.

Ah must be the other mp_ctx that Mac defaults to. Should we be worried though that is not being passed?

It's not super important at the moment I think. The parallel sampling should still work because sampling state is not passed around there, but if you cherry-pick that other commit, then things should pass.

@ricardoV94
Copy link
Member Author

ricardoV94 commented Jan 7, 2025

It was the sequential sampling (on the CI we only have 1 core). I had to reorder so we handle the default values before we do the fix on sequential sampling

@ricardoV94 ricardoV94 requested a review from lucianopaz January 7, 2025 19:37
Copy link
Contributor

@lucianopaz lucianopaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link

codecov bot commented Jan 7, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.85%. Comparing base (5d51953) to head (9dcd50a).
Report is 14 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #7637      +/-   ##
==========================================
+ Coverage   92.83%   92.85%   +0.01%     
==========================================
  Files         106      106              
  Lines       17748    17771      +23     
==========================================
+ Hits        16477    16501      +24     
+ Misses       1271     1270       -1     
Files with missing lines Coverage Δ
pymc/sampling/mcmc.py 86.18% <100.00%> (+0.05%) ⬆️
pymc/sampling/parallel.py 88.73% <100.00%> (ø)
pymc/step_methods/compound.py 97.56% <100.00%> (ø)
pymc/step_methods/hmc/quadpotential.py 84.69% <100.00%> (+0.06%) ⬆️
pymc/step_methods/state.py 96.55% <100.00%> (-1.57%) ⬇️
pymc/util.py 83.45% <100.00%> (+1.59%) ⬆️

@ricardoV94 ricardoV94 merged commit 82716fb into pymc-devs:main Jan 7, 2025
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sampling with generators as seeding no longer deterministic
2 participants